Skip to content

Comments

Mk/review/as/dev/installation completed event listener#5

Merged
printminion-co merged 23 commits intoas/dev/installation_completed_event_listenerfrom
mk/review/as/dev/installation_completed_event_listener
Jan 22, 2026
Merged

Mk/review/as/dev/installation completed event listener#5
printminion-co merged 23 commits intoas/dev/installation_completed_event_listenerfrom
mk/review/as/dev/installation_completed_event_listener

Conversation

@printminion-co
Copy link
Collaborator

No description provided.

…ner and related stubs

refactor: extract magic strings to class constants

Replace hardcoded magic strings with class constants for better maintainability:
- PostSetupJob: JOB_STATUS_DONE, JOB_STATUS_UNKNOWN, JOB_STATUS_CONFIG_KEY
- InstallationCompletedEventListener: JOB_STATUS_INIT, JOB_STATUS_CONFIG_KEY, ADMIN_CONFIG_PATH, ADMIN_USER_KEY

This improves code readability and reduces the risk of typos when using these values throughout the codebase.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ner and related stubs

refactor: use InstallationCompletedEvent properties instead of file parsing

Remove initAdminUser() method and config file reading logic in favor of using the event's built-in getAdminUsername() method. This eliminates the need for:
- Reading and parsing /vault/secrets/adminconfig
- Quote stripping logic
- ADMIN_CONFIG_PATH and ADMIN_USER_KEY constants

Updated stub to match the real implementation from nextcloud/server#57522 including:
- Constructor with dataDirectory, adminUsername, and adminEmail parameters
- Getter methods: getDataDirectory(), getAdminUsername(), getAdminEmail()
- hasAdminUser() helper method

Benefits:
- Cleaner code with no file I/O operations
- No error handling needed for missing/malformed config files
- Uses official Nextcloud API instead of custom parsing
- Better type safety with proper event typing

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

refactor: simplify isUrlAvailable method

Extract status code to a variable to avoid duplicate method calls and improve readability.

Changed from:
  return response.getStatusCode() >= 200 && response.getStatusCode() < 300;

To:
  statusCode = response.getStatusCode();
  return statusCode >= 200 && statusCode < 300;

Benefits:
- Avoids redundant method call
- Slightly better performance
- Clearer intent with named variable

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

refactor: remove redundant null check and improve control flow

Changed sendInitialWelcomeMail to use early return pattern when user doesn't exist, removing the redundant null check after userExists().

The userManager.get() can still return null in edge cases (e.g., user deleted between exists check and get), so we keep that null check but simplify the control flow.

Benefits:
- Clearer intent with early returns
- Job will now retry if user doesn't exist (doesn't mark as DONE)
- Removes unnecessary else block
- More consistent error handling

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

refactor: inject ITimeFactory via DI instead of direct instantiation

Replace direct instantiation of TimeFactory with dependency injection using ITimeFactory interface in WelcomeMailHelper.

Changed from:
  new TimeFactory()

To:
  Constructor parameter: private ITimeFactory timeFactory
  Usage: this->timeFactory

Benefits:
- Better testability with mock injection
- Follows dependency injection pattern consistently
- Uses interface instead of concrete implementation
- Aligns with Nextcloud DI container best practices

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

fix: add validation for overwrite.cli.url configuration

Add check to ensure overwrite.cli.url is configured before attempting to use it.

If the URL is not set or empty, the job will log a warning and return early, allowing the job to retry on the next cron run in case the configuration is added later.

This prevents potential issues with empty URLs being passed to the HTTP client.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

refactor: improve logging with structured context in isUrlAvailable

Replace string concatenation with structured logging arrays for better observability:
- Use context arrays instead of string concatenation
- Change exception logging from debug to info level (more appropriate for network errors)
- Improve log message wording: 'Checking URL availability' instead of 'Check URL:'
- Add structured context: url and exception message

Benefits:
- Better log aggregation and filtering in production
- Proper log level for transient network issues
- Easier to parse and analyze logs

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

refactor: improve log messages with context and appropriate levels

Enhanced logging throughout PostSetupJob with structured context and proper log levels:

Log level improvements:
- Changed job completion to info (was debug) - important milestone
- Changed job status unknown to warning (was debug) - indicates potential issue
- Changed URL unavailability to info (was debug) - transient expected condition

Message improvements:
- 'Post-installation job' instead of 'Post install job' for consistency
- 'Admin user not found, cannot send welcome email' instead of 'Could not find install user, skip sending welcome mail'
- 'System not ready, will retry' instead of 'domain is not ready yet, retry with cron until...'
- 'System URL not configured' instead of 'overwrite.cli.url is not configured'

Added structured context:
- adminUserId in all relevant log messages
- config_key for configuration errors
- url for availability checks

Benefits:
- Better log filtering and analysis in production
- Clearer user-facing messages
- Consistent terminology (email vs mail, system vs domain)
- Proper log levels for different scenarios

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

fix: handle null user retrieval and prevent marking job as done

Fix edge case where userManager.get() returns null after userExists() check.

Previously, if the user was deleted between userExists() and get() calls, the job would still mark itself as DONE without sending the email. Now the job will:
- Log an error message with context
- Return early without marking as DONE
- Retry on next cron run

This ensures the welcome email is eventually sent if the user becomes available again.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ner and related stubs

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ner and related stubs

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… functionality

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Add EditorConfig file to maintain consistent coding styles across
different editors and IDEs. Configures indent styles for PHP (tabs),
JSON/YAML (spaces), and other file types.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Add psalm-baseline.xml to track suppressed static analysis issues.
The baseline is currently empty as all issues are properly annotated
with @psalm-suppress directives.

Existing suppressions are kept because they are legitimate:
- PossiblyUnusedMethod: Constructors and methods called by DI container
- UndefinedClass: Internal Nextcloud classes not in stubs
- MixedAssignment/MixedMethodCall: Unavoidable when using internal classes

Baseline file enables tracking if new issues are introduced while
maintaining existing suppressions.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…onfig

Add stub for OCA\Settings\Mailer\NewUserMailHelper based on Nextcloud
server stable31 version. This eliminates UndefinedClass, MixedAssignment,
and MixedMethodCall errors, improving type inference to 100%.

Move all psalm suppressions from inline @psalm-suppress annotations to
centralized issueHandlers in psalm.xml, following Nextcloud best practices
as demonstrated in the mail app.

Benefits:
- 100% type inference (up from 99.3151%)
- Cleaner code without annotation clutter
- Centralized configuration easier to maintain
- Consistent with Nextcloud ecosystem standards

Stub source:
https://github.com/nextcloud/server/blob/stable31/apps/settings/lib/Mailer/NewUserMailHelper.php

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Enhanced WelcomeMailHelper tests to properly verify NewUserMailHelper
interaction and behavior:

- Verify password reset token generation and storage when requested
- Verify no token generation when not requested
- Verify email sending through NewUserMailHelper.sendMail()
- Verify no email sent to users without email addresses
- Added proper expectations for l10nFactory.getUserLanguage()
- Added proper expectations for timeFactory.getTime() and config.setUserValue()

Test coverage increased from 3 tests with 7 assertions to 4 tests with
11 assertions. All tests verify actual NewUserMailHelper behavior through
the stub, not just execution without exceptions.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Consolidated test setup and removed redundant mock configurations:

- Moved all mock setup into setUp() for better organization
- Simplified mock return values (e.g., willReturnArgument(0) for translations)
- Removed duplicate mock configurations across tests
- Kept only essential assertions that verify actual behavior
- Reduced code duplication while maintaining test coverage

Tests still verify:
- Password reset token generation when requested
- No token generation when not requested
- Email sending through NewUserMailHelper
- No email sent to users without email addresses

All 17 tests passing with 47 assertions.

Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
@printminion-co printminion-co merged commit 57b7346 into as/dev/installation_completed_event_listener Jan 22, 2026
17 of 28 checks passed
@printminion-co printminion-co deleted the mk/review/as/dev/installation_completed_event_listener branch January 23, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant